Conversation
Co-authored-by: Andreas Arvidsson <andreas.arvidsson87@gmail.com>
Co-authored-by: Andreas Arvidsson <andreas.arvidsson87@gmail.com>
Co-authored-by: Andreas Arvidsson <andreas.arvidsson87@gmail.com>
Co-authored-by: Andreas Arvidsson <andreas.arvidsson87@gmail.com>
AndreasArvidsson
left a comment
AndreasArvidsson
left a comment
There was a problem hiding this comment.
I've gone through all your new tests and left comments. Please use typescript as a reference language to help you figure out the correct ranges. We do have a scope visualizer on the docs page to help you with this:
Per language:
https://www.cursorless.org/docs/user/languages/typescript/
Per scope:
https://www.cursorless.org/docs/contributing/scopes/argumentList/
The tests are also failing which you need have a look at.
There was a problem hiding this comment.
bbbis missing as a scope- The removal range of
aaais incorrect - The insertion delimiter is incorrect for a multiline argument
There was a problem hiding this comment.
Please use another language, eg javascript, as a reference:
https://github.com/cursorless-dev/cursorless/blob/d21522be750a55b70b50ac251300bfd6dd4baace/data/fixtures/scopes/javascript.core/argument/argument.actual.multiLine.scope
There was a problem hiding this comment.
bbbis missing as a scope- The removal range of
aaais incorrect - The insertion delimiter is incorrect
There was a problem hiding this comment.
Please include a second argument so we get a separate removal range in this scope test
| >-----< | ||
| 0| foo(); | ||
|
|
||
| [Insertion delimiter] = " " |
There was a problem hiding this comment.
The insertion delimiter should be an empty string
| >-------------< | ||
| 0| foo(aaa, bbb); | ||
|
|
||
| [Insertion delimiter] = " " |
There was a problem hiding this comment.
The insertion delimiter should be ", "
|
|
||
| [#3 Removal] = 0:14-0:23 | ||
| >---------< | ||
| 0| fn foo(aaa: u8, bbb: u8) void {} |
| @@ -0,0 +1,62 @@ | |||
| fn foo(aaa: u8, bbb: u8) void {} | |||
There was a problem hiding this comment.
Please remove the arguments for this tests. Now the file is unnecessarily complicated.
|
|
||
| [Removal] = 0:11-0:14 | ||
| >---< | ||
| 0| const foo: u8 = 0; |
There was a problem hiding this comment.
Removal range should be : u8
| @@ -0,0 +1,20 @@ | |||
| const foo = "bar"; | |||
There was a problem hiding this comment.
Assignment should be assigning a value to an already existing variable. Please rename this file to value.variable
|
|
||
| [Removal] = 0:11-0:17 | ||
| >------< | ||
| 0| const foo = "bar"; |
There was a problem hiding this comment.
Removal range should be = "bar"
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ed6c426bb2
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| foo( | ||
| aaa, | ||
| bbb | ||
| ); |
There was a problem hiding this comment.
This fixture is named argumentList.formal.multiLine, but the sample code is a call expression (foo(...)) rather than a function declaration. The Zig query for formal argument lists only matches function_declaration (parameters), so this fixture won’t exercise the intended scope and can let regressions slip or produce misleading docs/tests for formal lists. Consider changing the snippet to a fn foo(... ) declaration so the fixture actually validates the formal list behavior.
Useful? React with 👍 / 👎.
| [#2 Content] = 0:12-0:14 | ||
| >--< | ||
| 0| fn foo(aaa: u8, bbb: u8) void {} |
There was a problem hiding this comment.
The type.return fixture includes additional captures for the parameter types ([#2 Content] = 0:12-0:14, etc.). That makes the fixture assert that parameter types are part of the return-type scope, which contradicts the facet’s intent and will either fail tests or mask a broken return-type query by validating the wrong targets. The fixture should only contain the return type range.
Useful? React with 👍 / 👎.
|
Sorry I haven't gotten to this yet — life threw a few curveballs at me at once. I really appreciate the thorough reviews and hopefully I'll be able to get back to this and wrap it up soon 🙏 |
|
No stress. Let's revisit whenever you have the time. |
3 participants
First slice of zig working end to end.
Let me know if there is anything I should change, e.g. should I make the query less specific? I have never used tree-sitter queries before